-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DYN-7535 Record python engine package information in graphs when using engines served from them #15515
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7535
To me - on first glance, the approach seems a bit backward. I am especially leary of the use of App domain because that doesn't really exist anymore, it just points to the default ALC - but as you know packages might be loaded into non default ALCs. I think a less brittle approach could be
|
|
packageBinPaths = packageBinPaths.Concat(Directory.GetFiles(ppath, "*.dll", SearchOption.AllDirectories)).ToList(); | ||
} | ||
|
||
var dotNetRuntimePaths = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
executing these calls over and over is a waste - disk access can be slow.
MetadataLoadContext mlc = null; | ||
var resolver = new PathAssemblyResolver(standardPaths); | ||
mlc = new MetadataLoadContext(resolver); | ||
var all = AppDomain.CurrentDomain.GetAssemblies().ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about this because you'll only find assemblies loaded into the default ALC, I don't think this will let you see all the assemblies loaded into the current process. You may need to dig around in the dotnet source to confirm or deny that.
AppDomain
is now just a facade on top of assembly load contexts.
} | ||
|
||
//ignoring assembly version | ||
private bool IsEqualPythonEngineAssemblyName(Type t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you found that this failed if the base types were of different versions? You cannot just check that the type is assignable to a PythonEngine
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assembly returned from MLC is different from the assembly loaded into the App, it had partial information, and the isAssignable check to PythonEngine
type did not resolve (even though I can see the same types), that is why I had to go for this comparison.
Somewhat related: https://stackoverflow.com/questions/10439668/getting-custom-assembly-attributes-without-loading-into-current-appdomain
if (a != null) | ||
{ | ||
//Fetch it's loaded instance from AppDomain | ||
var b = all.FirstOrDefault(x => x.GetName().Name.ToLower().Equals(Path.GetFileNameWithoutExtension(mlcassem.GetName().Name).ToLower()), null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be very slow.
{ | ||
return enginePackageName; | ||
} | ||
set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely weird that this is a public setter.
{ | ||
return enginePackageVersion; | ||
} | ||
set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
{ | ||
if (Instance.PythonEnginePackageMap.Count > 0) | ||
{ | ||
if (Instance.PythonEnginePackageMap.TryGetValue(engineName, out var tuple)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens when there are engines with the same name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add a check
@@ -222,36 +258,35 @@ private void LoadPythonEngine(Assembly assembly) | |||
PropertyInfo instanceProp = null; | |||
try | |||
{ | |||
//eType = assembly.GetTypes().FirstOrDefault(x => typeof(PythonEngine).IsAssignableFrom(x) && !x.IsInterface && !x.IsAbstract); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??
this PR should have some tests asserting that all of our python packages (or mock ones that mimic them) are identified correctly. |
RaisePropertyChanged(nameof(EngineName)); | ||
} | ||
} | ||
} | ||
|
||
[JsonProperty("EnginePackageName")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there value in serializing this with each node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same graph containing multiple python nodes, pointing to different engines, imported from different packages.
@zeusongit I’ll take a look at this tonight. The python engine APIs are still somewhat WIP and in the past proves to be tricky to get right |
@@ -125,8 +124,10 @@ internal static readonly Lazy<PythonEngineManager> | |||
[Obsolete("AvailableEngines field will be replaced in a future Dynamo release.")] | |||
public ObservableCollection<PythonEngine> AvailableEngines; | |||
|
|||
internal Dictionary<string, Tuple<string, string>> PythonEnginePackageMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not add the Package and Version properties to the PythonEngine class ?
All python engines are loaded from DSCPython
(part of Dynamo) or from the PackageManager.PackageLoader.TryLoadPackageIntoLibrary
. You could easily send the pkg information through the LoadPythonEngine
arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it is not straight-forward, the python engine is loaded as an extension, and extensions are loaded after packages, so we do not know if an engine was added after the packages are loaded.
{ | ||
enginePackageName = pkgDetails.Item1; | ||
enginePackageVersion = pkgDetails.Item2; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that all engine names stored throughout Dynamo should be changed to something like FullyQualifiedEngineName (PackageName.EngineName@PkgVersion) So we should not need to match the EngineName to packages.
Is this change motivated by the fact that we can now load multiple versions of the same Python Package in the same dynamo session ? (because of assembly isolation)...? |
|
||
private void PopulatePythonEnginePackageMap() | ||
{ | ||
var packageBinPaths = new List<string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you doing all this assembly searching and loading here ?
PythonEngines are already loaded In the PythonEngineManager constructor and in the PackageManager.PackageLOader.
The PythonEnginesList
from this class will always be kept up to date when PythonEngineManager.Instance.AvailableEngines
is changed. We should be able to store all the info we need in the PythonENgine classes when they are initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you doing all this assembly searching and loading here ?
Had to do all this to avoid breaking changes, also the idea was to not depend on the extension/engine to tell what package they come from (at-least for me), if allowed to change PythonEngine, or add a dependency on the extension itself to declare which package they belong to, it would have been way simpler. I agree.
Hi @pinzart90 , |
So the problem is that newer versions of a PythonPackage are not/expected to not be backward compatible and thus causing graphs to break ? |
Yes, it could considering the current state, that has some bugs which when fixed in the future version, may break graphs. Also, the other way around, when a user saves the graph with a newer version, and then shares the graph to a user who has an older version installed. |
We could very well just store all the loaded packages (name, version etc) in the graph and then each serialized node could point to one of those packages (so all nodes could restore specific versions of packages). Or does this already exist in some form ? |
I think because the PythonNode itself is not dependent on the package, but the engine it uses could be. |
I think we could make workspaceDEpendency system work python python nodes. I would vote to use that system since it is already doing the same thing for other nodes |
It would probably need to be adapted to take into account AssemblyLoadContexts and the ability of the python node to change its |
Yes, that is the second part of this task, to use this information and trigger workspace dependency flow (not yet implemented) |
YEah but the workspaceDEpendency system already serializes the package version information in the graph. I do not see why we would want to duplicate that information with another custom serialization of the python node models |
Yes I need to look into ALC and yes for dynamic changing package info. |
Ok, got it, I can look into reusing |
Yes, you can add internal data to that class |
But I am not sure you will need too, since the PakcageManagerExtension already has |
Alternate implementation: #15530 PTAL |
Purpose
The PR adds package information to dynamo graphs consisting of python nodes using the latest engine.
When a python engine is served from a package, the package name and version information is lost, due to which we were unable to display dependency mismatch in workspace references. This PR will preserve package name and version information as part of the python node that uses a python engine served from a package.
Following steps are performed to populate a map of python engine and it's respective package name and version:
PythonEngine
.(Note: I know variable name are a bit vague, will update them as the review goes on)
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Reviewers
@DynamoDS/dynamo